[Prototype] Retries for Storage #197
Conversation
googlebot
added
the
cla: yes
frankyn
added
the
kokoro:force-run
frankyn
changed the title
yoshi-kokoro
removed
the
kokoro:force-run
| @@ -134,6 +136,14 @@ def _query_params(self): | |||
| params["userProject"] = self.user_project | |||
| return params | |||
|
|
|||
|
|
|||
| def _call_api(self, client, retry, **kwargs): | |||
| call = functools.partial(client._connection.api_request, **kwargs) | |||
There was a problem hiding this comment.
Is this because the interface for the retry function is not expected to pass in any arguments?
There was a problem hiding this comment.
Copied it over mainly:
At the moment no, I'm not making it configurable for the first pass and only introducing it to help support/unblock customer friction when using the library.
| import urllib3 | ||
|
|
||
| from google.api_core import exceptions | ||
| from google.api_core import retry |
There was a problem hiding this comment.
Neat, so this is what we told customers to use as a workaround already right?
There was a problem hiding this comment.
Also, where are configs around number of attempts or timeouts? Are these values hard-coded into google.api_core.retry?
| reason = exc.errors[0]["reason"] | ||
|
|
||
| return reason in _RETRYABLE_REASONS | ||
| else: |
There was a problem hiding this comment.
No need for else here right? And the nested if statements and uses of isinstance look pretty ugly, I'm sure there's a nicer way to do this.
| return isinstance(exc, _UNSTRUCTURED_RETRYABLE_TYPES) | ||
| reason = exc.errors[0]["reason"] | ||
|
|
||
| return reason in _RETRYABLE_REASONS |
There was a problem hiding this comment.
Do you not have access to response codes here? Seems kind of a strange workaround to be doing string matching for a 503 for example.
| return True | ||
| return False | ||
|
|
||
| _DEFAULT_RETRY = retry.Retry(predicate=_should_retry) |
There was a problem hiding this comment.
I would be curious to look at other examples of where people have written these predicates.
| @@ -134,6 +136,14 @@ def _query_params(self): | |||
| params["userProject"] = self.user_project | |||
| return params | |||
|
|
|||
|
|
|||
| def _call_api(self, client, retry, **kwargs): | |||
There was a problem hiding this comment.
Should retry be a kwarg? Do callers explicitly have to pass in None if they don't want a retry? Would be good to clarify here.
|
Is there any chance of this being merged soon? We'd very much like to have this functionality available in the library, we currently have to wrap everything in retries ourselves. |
|
@busunkim96 Why was this closed? Is there a new PR somewhere else? |
|
I'll be working on a new one. |
frankyn
added
the
do not merge
|
Is there any update on this? We recently encountered no retries for |
|
Closing this PR as @andrewsg is working on a new PR now. PR should be available soon. Thank you for your patience. |
Prototype based on BigQuery retry implementation in Python: https://github.com/googleapis/python-bigquery/blob/master/google/cloud/bigquery/retry.py
Work towards resolving: #108
Remaining: